Integrate themed icon runtime and fix GTK import issues#46
Integrate themed icon runtime and fix GTK import issues#46xcrsz wants to merge 3 commits intoghostbsd:masterfrom
Conversation
Reviewer's GuideIntegrates a thread-safe, GTK3-pinned themed icon resolution runtime (with async/sync APIs and legacy XPM fallback), reorganizes mapping/indexing into dedicated modules, cleans up iconlist imports and formatting, and updates packaging to include the new helpers and document missing dependencies. Sequence diagram for async themed icon and label resolutionsequenceDiagram
participant Caller
participant IconRuntime
participant WorkerThread
participant MainThread
Caller->>IconRuntime: themed_icon_and_label_async(category, pkg_name, size, on_ready)
IconRuntime->>WorkerThread: _resolve_label_and_icon_name_worker(pkg_name, curated_map)
WorkerThread->>MainThread: main_thread_finish() via GLib.idle_add
MainThread->>IconRuntime: _load_icon_pixbuf_main(icon_name, size)
MainThread->>Caller: on_ready(label, pixbuf)
Class diagram for new themed icon runtime and helpersclassDiagram
class IconRuntime {
+init_icon_runtime()
+resolve_label_and_icon_async(pkg_name, curated_map, size, on_ready)
+resolve_label_and_icon_sync(pkg_name, curated_map, size)
}
class DesktopIndex {
+build_index_async()
+wait_until_ready(timeout)
+best_guess(token)
}
class PkgDesktopMap {
+build_pkg_map_async()
+desktop_for_pkg(pkg)
}
class AccessoriesMap {
<<dict>>
}
IconRuntime --> DesktopIndex : uses
IconRuntime --> PkgDesktopMap : uses
IconRuntime --> AccessoriesMap : uses
DesktopIndex <.. PkgDesktopMap : used for lookup
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
Blocking issues:
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
General comments:
- Your setup.py lists 'software_station' as a package but there’s no init.py in that directory, so your new modules won’t be installed – please add an init.py or adjust the packaging.
- This PR introduces multiple large features (themed icon runtime, desktop‐entry indexing, pkg→desktop mapping, packaging tweaks); consider splitting it into smaller, focused PRs for easier review and validation.
- The ThreadPoolExecutor
_executornever shuts down on app exit, which can leave non‐daemon threads running; consider adding a cleanup hook or marking threads as daemons to avoid resource leaks.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Your setup.py lists 'software_station' as a package but there’s no __init__.py in that directory, so your new modules won’t be installed – please add an __init__.py or adjust the packaging.
- This PR introduces multiple large features (themed icon runtime, desktop‐entry indexing, pkg→desktop mapping, packaging tweaks); consider splitting it into smaller, focused PRs for easier review and validation.
- The ThreadPoolExecutor `_executor` never shuts down on app exit, which can leave non‐daemon threads running; consider adding a cleanup hook or marking threads as daemons to avoid resource leaks.
## Individual Comments
### Comment 1
<location> `iconlist.py:63-72` </location>
<code_context>
+def themed_icon_and_label_async(
</code_context>
<issue_to_address>
**suggestion (performance):** Synchronous callback in non-themed path may block if legacy_get_pixbuf is slow.
To avoid blocking the main thread, offload legacy_get_pixbuf to a worker thread and use GLib.idle_add for the callback, as done in the themed path.
Suggested implementation:
```python
import threading
from gi.repository import GLib
def themed_icon_and_label_async(
category: str,
pkg_name: str,
size: int,
on_ready: Callable[[str, Optional["GdkPixbuf.Pixbuf"]], None],
) -> None:
"""
Resolve a label + icon without blocking the UI.
- If themed stack is available and enabled for the category, use it.
- Otherwise, offload legacy_get_pixbuf to a worker thread and use GLib.idle_add for the callback.
The callback runs on the GTK main thread.
```
```python
# Use the themed stack if available and enabled for the category
if _category_uses_themed(category):
# ... themed path ...
# (existing async code for themed path)
return
# Non-themed path: offload legacy_get_pixbuf to a worker thread
def worker():
pixbuf = legacy_get_pixbuf(pkg_name, size)
GLib.idle_add(on_ready, pkg_name, pixbuf)
threading.Thread(target=worker, daemon=True).start()
```
</issue_to_address>
### Comment 2
<location> `software_station/icons.py:33-34` </location>
<code_context>
+
+# Small worker pool: enough parallelism without hammering disks
+from concurrent.futures import ThreadPoolExecutor
+_executor = ThreadPoolExecutor(max_workers=3)
+
+def _assert_main_thread():
</code_context>
<issue_to_address>
**suggestion (bug_risk):** ThreadPoolExecutor is created at module level and never shut down.
Please ensure the executor is properly shut down on application exit to prevent resource leaks.
```suggestion
from concurrent.futures import ThreadPoolExecutor
import atexit
_executor = ThreadPoolExecutor(max_workers=3)
atexit.register(_executor.shutdown)
```
</issue_to_address>
### Comment 3
<location> `software_station/icons.py:94` </location>
<code_context>
+ ):
+ pkg_desktop_map.build_pkg_map_async()
+
+def _load_icon_pixbuf_main(icon_name: Optional[str], size: int = 32) -> Optional[GdkPixbuf.Pixbuf]:
+ """
+ Main-thread ONLY. HiDPI-aware with robust fallback. Cached.
</code_context>
<issue_to_address>
**suggestion:** Icon loading logic may not handle missing or corrupt icon themes gracefully.
If no icon is loaded due to a missing or corrupt theme, log a warning to help diagnose theme issues.
Suggested implementation:
```python
import logging
def _load_icon_pixbuf_main(icon_name: Optional[str], size: int = 32) -> Optional[GdkPixbuf.Pixbuf]:
"""
Main-thread ONLY. HiDPI-aware with robust fallback. Cached.
"""
_assert_main_thread()
assert _icon_theme is not None
key = ((icon_name or ICON_FALLBACK), size, _scale)
if key in _pixbuf_cache:
return _pixbuf_cache[key]
name = icon_name or ICON_FALLBACK
pix: Optional[GdkPixbuf.Pixbuf] = None
```
```python
name = icon_name or ICON_FALLBACK
pix: Optional[GdkPixbuf.Pixbuf] = None
# ... (icon loading logic here, not shown in this snippet) ...
if pix is None:
logging.warning(
"Icon '%s' could not be loaded (missing or corrupt theme?)",
name
)
```
If the icon loading logic is not shown in this snippet, ensure that the warning is placed immediately after all attempts to load the icon, before returning or caching the result.
If your project uses a custom logger, replace `logging.warning` with your project's logging convention.
</issue_to_address>
### Comment 4
<location> `software_station/desktop_index.py:50-59` </location>
<code_context>
+def build_index_async():
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Desktop index is built asynchronously but no error reporting is provided.
Please add error logging or a status callback to surface failures during index building, such as missing directories or permission issues.
Suggested implementation:
```python
import logging
logger = logging.getLogger(__name__)
def build_index_async(status_callback=None):
def work():
try:
locale = os.environ.get("LC_ALL") or os.environ.get("LANG") or "en_US"
for base in _DESKTOP_DIRS:
for path in glob(os.path.join(base, "*.desktop")):
try:
kf = GLib.KeyFile()
kf.load_from_file(path, GLib.KeyFileFlags.NONE)
if not kf.has_group("Desktop Entry"):
continue
except Exception as e:
logger.error(f"Failed to process desktop file '{path}': {e}", exc_info=True)
if status_callback:
status_callback(f"Error processing '{path}': {e}")
except Exception as e:
logger.error(f"Error during index building: {e}", exc_info=True)
if status_callback:
status_callback(f"Index build failed: {e}")
```
If you are running this asynchronously (e.g., in a thread), ensure that the `work()` function is started appropriately and that the `status_callback` is passed from the caller. You may also want to configure the logging output elsewhere in your application.
</issue_to_address>
### Comment 5
<location> `software_station/pkg_desktop_map.py:33-42` </location>
<code_context>
+def build_pkg_map_async():
</code_context>
<issue_to_address>
**suggestion (performance):** Subprocess calls in build_pkg_map_async may be slow for large package sets.
Consider batching, parallelizing, or caching subprocess calls to improve performance with large package sets.
Suggested implementation:
```python
import concurrent.futures
_ready = threading.Event()
def _run(cmd: list[str]) -> str:
"""Run a command, return stdout as text; swallow errors and stderr."""
try:
cp = subprocess.run(cmd, capture_output=True, text=True, check=False)
return cp.stdout or ""
except Exception:
return ""
def build_pkg_map_async():
"""Build the pkg→desktop map in a background thread."""
def work():
try:
# All installed package names, one per line
pkgs_txt = _run(["pkg", "query", "%n"])
if not pkgs_txt:
return
pkgs = [p for p in pkgs_txt.splitlines() if p]
def process_pkg(pkg):
# Replace this with the actual subprocess logic for each package
# For example: return _run(["pkg", "query", ...])
return pkg, _run(["pkg", "query", "%n", pkg])
with concurrent.futures.ThreadPoolExecutor() as executor:
results = list(executor.map(process_pkg, pkgs))
# Now results contains tuples of (pkg, output)
for pkg, output in results:
# Process output as needed
pass
```
You will need to replace the placeholder in `process_pkg` with the actual subprocess logic for each package, as the partial code does not show what is being queried per package. Also, update the result processing loop to handle the outputs as required by your application.
</issue_to_address>
### Comment 6
<location> `software_station/pkg_desktop_map.py:28` </location>
<code_context>
cp = subprocess.run(cmd, capture_output=True, text=True, check=False)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 7
<location> `software_station/icons.py:110-112` </location>
<code_context>
info = _icon_theme.lookup_icon_for_scale(name, size, _scale, 0)
if not info:
info = _icon_theme.lookup_icon_for_scale(ICON_FALLBACK, size, _scale, 0)
</code_context>
<issue_to_address>
**suggestion (code-quality):** Use `or` for providing a fallback value ([`use-or-for-fallback`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/use-or-for-fallback))
```suggestion
info = _icon_theme.lookup_icon_for_scale(name, size, _scale, 0) or _icon_theme.lookup_icon_for_scale(ICON_FALLBACK, size, _scale, 0)
```
<br/><details><summary>Explanation</summary>Thanks to the flexibility of Python's `or` operator, you can use a single
assignment statement, even if a variable can retrieve its value from various
sources. This is shorter and easier to read than using multiple assignments with
`if not` conditions.
</details>
</issue_to_address>
### Comment 8
<location> `software_station/icons.py:59` </location>
<code_context>
def init_icon_runtime():
"""
Call ONCE at app startup (main thread).
- creates global IconTheme
- wires watchers for theme & DPI changes
- optionally kicks off indexing helpers
"""
_assert_main_thread()
global _icon_theme
_icon_theme = Gtk.IconTheme.get_default()
# Theme watcher
settings = Gtk.Settings.get_default()
if settings:
settings.connect("notify::gtk-icon-theme-name", _on_icon_theme_change)
# DPI / monitor changes
display = Gdk.Display.get_default()
if display:
display.connect("monitor-added", _rebuild_scale_and_clear_cache)
display.connect("monitor-removed", _rebuild_scale_and_clear_cache)
_rebuild_scale_and_clear_cache()
# Optional background indices
if desktop_index and hasattr(desktop_index, "build_index_async"):
desktop_index.build_index_async()
# Optional: build pkg → .desktop map, unless explicitly disabled
if (
pkg_desktop_map
and hasattr(pkg_desktop_map, "build_pkg_map_async")
and os.environ.get("SOFTWARE_STATION_DISABLE_PKG_MAP") != "1"
):
pkg_desktop_map.build_pkg_map_async()
</code_context>
<issue_to_address>
**issue (code-quality):** Use named expression to simplify assignment and conditional [×2] ([`use-named-expression`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-named-expression/))
</issue_to_address>
### Comment 9
<location> `software_station/icons.py:115-119` </location>
<code_context>
def _load_icon_pixbuf_main(icon_name: Optional[str], size: int = 32) -> Optional[GdkPixbuf.Pixbuf]:
"""
Main-thread ONLY. HiDPI-aware with robust fallback. Cached.
"""
_assert_main_thread()
assert _icon_theme is not None
key = ((icon_name or ICON_FALLBACK), size, _scale)
if key in _pixbuf_cache:
return _pixbuf_cache[key]
name = icon_name or ICON_FALLBACK
pix: Optional[GdkPixbuf.Pixbuf] = None
try:
# Prefer lookup_icon_for_scale if present
if hasattr(_icon_theme, "lookup_icon_for_scale"):
info = _icon_theme.lookup_icon_for_scale(name, size, _scale, 0)
if not info:
info = _icon_theme.lookup_icon_for_scale(ICON_FALLBACK, size, _scale, 0)
if info:
pix = info.load_icon()
else:
if _icon_theme.has_icon(name):
pix = _icon_theme.load_icon(name, size, 0)
elif _icon_theme.has_icon(ICON_FALLBACK):
pix = _icon_theme.load_icon(ICON_FALLBACK, size, 0)
except Exception:
# best-effort final fallback
try:
if _icon_theme.has_icon(ICON_FALLBACK):
pix = _icon_theme.load_icon(ICON_FALLBACK, size, 0)
except Exception:
pix = None
_pixbuf_cache[key] = pix
return pix
</code_context>
<issue_to_address>
**suggestion (code-quality):** Merge else clause's nested if statement into elif ([`merge-else-if-into-elif`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/merge-else-if-into-elif/))
```suggestion
elif _icon_theme.has_icon(name):
pix = _icon_theme.load_icon(name, size, 0)
elif _icon_theme.has_icon(ICON_FALLBACK):
pix = _icon_theme.load_icon(ICON_FALLBACK, size, 0)
```
</issue_to_address>
### Comment 10
<location> `software_station/icons.py:194-197` </location>
<code_context>
def _resolve_label_and_icon_name_worker(pkg_name: str, curated_map: dict) -> tuple[str, Optional[str]]:
"""
Worker thread: decide (label, icon_name STRING). No GTK here.
Order:
1) curated_map
2) pkg → desktop (optional, via pkg_desktop_map → desktop_index)
3) desktop_index.best_guess(pkg_name)
4) fallback: pkg_name + None
"""
# 1) curated
info = curated_map.get(pkg_name, {})
friendly = info.get("name")
icon_name = info.get("icon")
# 2) pkg → desktop path → desktop_index
if (not friendly or not icon_name) and pkg_desktop_map and hasattr(pkg_desktop_map, "desktop_for_pkg"):
try:
desktop_path = pkg_desktop_map.desktop_for_pkg(pkg_name)
if desktop_path and desktop_index:
desktop_id = os.path.basename(desktop_path)
hit = desktop_index.best_guess(desktop_id)
if hit and isinstance(hit, dict):
if not friendly:
friendly = hit.get("name")
if not icon_name:
icon_name = hit.get("icon")
except Exception:
pass
# 3) direct desktop_index lookup
if not friendly or not icon_name:
name_guess = _friendly_name_guess(pkg_name)
icon_guess = _icon_name_guess(pkg_name)
if not friendly and name_guess:
friendly = name_guess
if not icon_name and icon_guess:
icon_name = icon_guess
# 4) fallback
if not friendly:
friendly = pkg_name
return (friendly, icon_name)
</code_context>
<issue_to_address>
**suggestion (code-quality):** Hoist conditional out of nested conditional [×2] ([`hoist-if-from-if`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/hoist-if-from-if/))
```suggestion
if not friendly and name_guess:
friendly = name_guess
if not icon_name and icon_guess:
icon_name = icon_guess
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| ): | ||
| pkg_desktop_map.build_pkg_map_async() | ||
|
|
||
| def _load_icon_pixbuf_main(icon_name: Optional[str], size: int = 32) -> Optional[GdkPixbuf.Pixbuf]: |
There was a problem hiding this comment.
suggestion: Icon loading logic may not handle missing or corrupt icon themes gracefully.
If no icon is loaded due to a missing or corrupt theme, log a warning to help diagnose theme issues.
Suggested implementation:
import logging
def _load_icon_pixbuf_main(icon_name: Optional[str], size: int = 32) -> Optional[GdkPixbuf.Pixbuf]:
"""
Main-thread ONLY. HiDPI-aware with robust fallback. Cached.
"""
_assert_main_thread()
assert _icon_theme is not None
key = ((icon_name or ICON_FALLBACK), size, _scale)
if key in _pixbuf_cache:
return _pixbuf_cache[key]
name = icon_name or ICON_FALLBACK
pix: Optional[GdkPixbuf.Pixbuf] = None name = icon_name or ICON_FALLBACK
pix: Optional[GdkPixbuf.Pixbuf] = None
# ... (icon loading logic here, not shown in this snippet) ...
if pix is None:
logging.warning(
"Icon '%s' could not be loaded (missing or corrupt theme?)",
name
)If the icon loading logic is not shown in this snippet, ensure that the warning is placed immediately after all attempts to load the icon, before returning or caching the result.
If your project uses a custom logger, replace logging.warning with your project's logging convention.
| _pixbuf_cache.clear() | ||
|
|
||
| def init_icon_runtime(): | ||
| """ |
There was a problem hiding this comment.
issue (code-quality): Use named expression to simplify assignment and conditional [×2] (use-named-expression)
| if not friendly and name_guess: | ||
| friendly = name_guess | ||
| if not icon_name and icon_guess: | ||
| icon_name = icon_guess |
There was a problem hiding this comment.
suggestion (code-quality): Hoist conditional out of nested conditional [×2] (hoist-if-from-if)
| if not friendly and name_guess: | |
| friendly = name_guess | |
| if not icon_name and icon_guess: | |
| icon_name = icon_guess | |
| if not friendly and name_guess: | |
| friendly = name_guess | |
| if not icon_name and icon_guess: | |
| icon_name = icon_guess |
|
Will give some time for others to review software before submitting a pull request. Software is located at: |
Implemented async themed icon resolution with graceful fallbacks:
package-x-generic as a fallback icon before async updates.
via themed_icon_and_label_async() and themed_icon_and_label_sync().
Introduced iconlist.py runtime enhancements:
to prevent “Namespace Gtk already loaded with version 4.0” errors.
Cleaned and formatted software-station:
Setup improvements:
(pkg install py311-distutils-extra) and provided fallback path.
Summary by Sourcery
Integrate a theme-aware, thread-safe icon runtime with async resolution and legacy fallbacks, refactor iconlist to use the new API, pin GTK 3.0 to avoid import conflicts, and update packaging to include the new modules
New Features:
Bug Fixes:
Enhancements:
Build:
Documentation: